-
Notifications
You must be signed in to change notification settings - Fork 194
feat: support grpc plugins with ts and bun #2293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support grpc plugins with ts and bun #2293
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds language-aware plugin generation/build/test flows (Go and TypeScript/Bun), replaces single goModulePath with structured ProtoOption[] across proto generation, introduces many TS and Go templates, updates toolchain to be language-aware, adds template compiler and CI steps, and adds a demo Bun plugin with tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
147-151: Apply language fallback when unsupported language is detectedThe guard at line 65 logs a fallback message but does not actually update
options.language. Add the assignment after the condition:if (options.language !== 'go' || options.language !== 'ts') { spinner.fail(pc.yellow(`Language '${options.language}' is not supported yet. Using 'go' instead.`)); + options.language = 'go'; }This ensures both the switch statement template selection (line 87) and the result display (line 147) reflect the fallback language.
🧹 Nitpick comments (20)
demo/pkg/subgraphs/student/.cursor/rules/plugin-development.mdc (1)
43-43: Clarify guidance on build verification.Line 43 states "Don't verify if the binary was created. The build command will take care of that," but this is ambiguous. Developers may be uncertain whether this means:
- The build will silently succeed even if the binary is missing
- Errors will be visible in build output
- They should trust the exit code
Consider clarifying the intent:
-6. Your job is done after successfully building the plugin. Don't verify if the binary was created. The build command will take care of that. +6. Your job is done after the build completes successfully. The build command will fail with an error if the binary cannot be created, so you can trust the exit code.cli/student/.cursor/rules/plugin-development.mdc (1)
1-85: Consider consolidating duplicate documentation.Both
.cursor/rules/plugin-development.mdcfiles (this one anddemo/pkg/subgraphs/student/.cursor/rules/plugin-development.mdc) contain identical content except for the directory paths. This duplication creates a maintenance burden: if the guidance needs to be updated, both files must be modified in sync to avoid divergence.Consider options:
- Move to a shared location: Store a single source-of-truth guide in a central docs directory and reference it from both
.cursor/rules/locations via symlink or inclusion.- Auto-generate: If these files are generated by a CLI command, ensure the generation logic produces consistent output across multiple plugin directories.
- Document the policy: If duplication is intentional (e.g., each plugin directory is self-contained), add a comment at the top of each file stating "This file is intentionally duplicated in [other locations] for local IDE guidance. Keep in sync when updating."
cli/student/Makefile (1)
2-2: Add standard phony targets "all" and "clean", and avoid shadowing "make" target.The Makefile should declare the standard phony targets "all" and "clean" per convention. Additionally, renaming the "make" target (line 7) to something like "default" or "setup" would avoid shadowing the built-in Make command. Consider:
-.PHONY: build test generate install-wgc +.PHONY: all build test generate install-wgc clean +all: build + +clean: + rm -rf dist/ node_modules/ + -make: build +default: buildcli/student/README.md (1)
23-34: Address markdown lint and align docs with TS/Bun scaffolding.
- Add a language to fenced code (MD040).
- Paths/instructions appear Go‑only; include TS/Bun equivalents and correct folder to
cli/student/if applicable.- ``` + ```text plugins/student/ @@ - │ ├── main.go # Main plugin implementation - │ ├── main_test.go # Tests for the plugin - │ └── schema.graphql # GraphQL schema defining the API + │ ├── main.go / plugin.ts # Go or TS implementation + │ ├── *_test.go / *.spec.ts # Tests + │ └── schema.graphql # GraphQL schema @@ - └── bin/ # Compiled binaries (created during build) - └── plugin # The compiled plugin binary + └── bin/ # Build output + └── plugin / dist # Go binary or TS build artifactsAlso extend “Customizing” with TS/Bun:
- TS:
bun install,bun run build, run client/server examples.- Go:
make generate && make test && make build.Also applies to: 38-41
demo/pkg/subgraphs/student/src/schema.graphql (1)
1-17: Confirm composition intent for Query.If this schema composes with other subgraphs, prefer
extend type Queryto avoid redefining the root. If it’s a standalone demo, current form is fine. Consider adding federation directives later ifWorldbecomes an entity.demo/pkg/subgraphs/student/src/plugin.ts (2)
3-6: Remove unused imports
pathandfileURLToPatharen’t used.-import * as path from 'path'; -import { fileURLToPath } from 'url';
63-64: Nit: prefer template stringsReadability/consistency.
- world.setId(`world-`+counter); - world.setName(`Hello There, `+ name); + world.setId(`world-${counter}`); + world.setName(`Hello There, ${name}`);demo/pkg/subgraphs/student/Makefile (1)
2-19: Make the default target build, add all/clean, and drop the “make” aliasCurrent first target (
install-wgc) runs on plainmake. Add anallalias, set the default goal, provideclean, and avoid global installs where possible.-.PHONY: build test generate install-wgc +.PHONY: all build test generate publish install-wgc clean +.DEFAULT_GOAL := build install-wgc: - @which wgc > /dev/null 2>&1 || npm install -g wgc@latest + @command -v wgc >/dev/null 2>&1 || npx -y wgc@latest --version >/dev/null -make: build +all: build test: install-wgc wgc router plugin test . generate: install-wgc wgc router plugin generate . -publish: generate +publish: build wgc router plugin publish . build: install-wgc wgc router plugin build . --debug + +clean: + @rm -rf dist .wgc || trueRun checkmake again to confirm warnings are resolved.
cli/student/src/plugin.ts (6)
3-6: Clean up unused imports and derived variables
path,fileURLToPath,__filename,__dirnameare unused; removing them also satisfies the import-order lint failures.-import * as grpc from '@grpc/grpc-js'; -import * as path from 'path'; -import { fileURLToPath } from 'url'; +import * as grpc from '@grpc/grpc-js'; @@ -// Get the directory of the current module -const __filename = fileURLToPath(import.meta.url); -const __dirname = path.dirname(__filename);Also applies to: 18-21
26-41: Logger doesn’t log — write to stderr to avoid polluting stdout handshakeEnsure logs go to stderr; keep stdout reserved for the go-plugin handshake line.
class Logger { log(level: string, message: string): void { const timestamp = new Date().toISOString(); + // Emit to stderr to not interfere with go-plugin stdout handshake + console.error(`[${timestamp}] [${level}] ${message}`); }
55-56: Remove placeholder text in greetingLooks accidental.
- world.setName(`Hello Thereeeee 17 17 17 17 17 17 17 17 1777 `+ name); + world.setName(`Hello There, ${name}`);
96-99: Fix typo and surface startup errors
s/serverL/server/and ensure the message is visible.- logger.error(`Failed to start serverL `+ error.message); + logger.error(`Failed to start server: ${error.message}`);
3-3: Satisfy lint: prefer node: specifier and import ordering (if you keep these imports)If you decide to keep
path/url, follow the lints: usenode:pathdefault import and place@grpc/grpc-jsafterurl.-import * as grpc from '@grpc/grpc-js'; -import * as path from 'path'; -import { fileURLToPath } from 'url'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; +import * as grpc from '@grpc/grpc-js';Also applies to: 4-4
60-62: Use template strings for readability- logger.info("Returning world: id="+world.getId()+", name="+world.getName()+", counter="+counter); + logger.info(`Returning world: id=${world.getId()}, name=${world.getName()}, counter=${counter}`);cli/src/commands/router/commands/plugin/templates/goplugin.ts (1)
15-19: Prefer Register{serviceName}Server for consistency with tests and idiomatic gRPCUse the generated registrar helper instead of
RegisterServicewithServiceDesc.- pl, err := routerplugin.NewRouterPlugin(func(s *grpc.Server) { - s.RegisterService(&service.{serviceName}_ServiceDesc, &{serviceName}{ - nextID: 1, - }) - }, routerplugin.WithTracing()) + pl, err := routerplugin.NewRouterPlugin(func(s *grpc.Server) { + service.Register{serviceName}Server(s, &{serviceName}{ + nextID: 1, + }) + }, routerplugin.WithTracing())cli/src/commands/router/commands/plugin/commands/init.ts (1)
85-97: Minor: add missing break and keep cases symmetricalNot strictly required (last case), but keeps future edits safe.
case 'ts': { await writeFile(resolve(srcDir, 'plugin.ts'), pupa(TsTemplates.pluginTs, { serviceName })); await writeFile(resolve(srcDir, 'client.ts'), pupa(TsTemplates.clientTs, { serviceName })); await writeFile(resolve(tempDir, 'package.json'), pupa(TsTemplates.packageJson, { serviceName })); + break; }cli/src/commands/router/commands/plugin/templates/tsplugin.ts (2)
97-104: Template polish: overly noisy greeting.The hardcoded “Hello Thereeeee 17…” looks accidental. Keep the template clean and predictable.
- world.setName(`Hello Thereeeee 17 17 17 17 17 17 17 17 1777 `+ name); + world.setName(`Hello ${name}`);
146-167: package.json template: versions reasonable; consider pinning @types/bun.To improve reproducibility, avoid "latest" on @types/bun.
- "@types/bun": "latest", + "@types/bun": "^1.1.10",cli/src/commands/router/commands/plugin/toolchain.ts (2)
248-296: Tool install checks always include Go; make language-aware to avoid unnecessary installs for TS-only builds.TS path only needs protoc; prompting to install Go increases friction.
- Accept a language argument in checkAndInstallTools and conditionally verify/install Go toolchain only when language === 'go'.
- Alternatively, split into checkAndInstallGoTools() and checkAndInstallCommonTools().
456-485: TS codegen path: robust; minor Windows chmod caveat.Good validation and plugin wiring. Note chmod on Windows is a no-op; you can guard by process.platform !== 'win32' to avoid confusing logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
cli/student/bun.lockis excluded by!**/*.lockcli/student/generated/mapping.jsonis excluded by!**/generated/**cli/student/generated/service.protois excluded by!**/generated/**cli/student/generated/service.proto.lock.jsonis excluded by!**/generated/**cli/student/generated/service_grpc_pb.d.tsis excluded by!**/generated/**cli/student/generated/service_grpc_pb.jsis excluded by!**/generated/**cli/student/generated/service_pb.d.tsis excluded by!**/generated/**cli/student/generated/service_pb.jsis excluded by!**/generated/**demo/pkg/subgraphs/student/bun.lockis excluded by!**/*.lockdemo/pkg/subgraphs/student/generated/mapping.jsonis excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service.protois excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service.proto.lock.jsonis excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service_grpc_pb.d.tsis excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service_grpc_pb.jsis excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service_pb.d.tsis excluded by!**/generated/**demo/pkg/subgraphs/student/generated/service_pb.jsis excluded by!**/generated/**
📒 Files selected for processing (29)
cli/src/commands/router/commands/plugin/commands/build.ts(3 hunks)cli/src/commands/router/commands/plugin/commands/generate.ts(2 hunks)cli/src/commands/router/commands/plugin/commands/init.ts(2 hunks)cli/src/commands/router/commands/plugin/templates/goplugin.ts(1 hunks)cli/src/commands/router/commands/plugin/templates/plugin.ts(0 hunks)cli/src/commands/router/commands/plugin/templates/tsplugin.ts(1 hunks)cli/src/commands/router/commands/plugin/toolchain.ts(5 hunks)cli/student/.cursor/rules/plugin-development.mdc(1 hunks)cli/student/.cursorignore(1 hunks)cli/student/.gitignore(1 hunks)cli/student/Dockerfile(1 hunks)cli/student/Makefile(1 hunks)cli/student/README.md(1 hunks)cli/student/package.json(1 hunks)cli/student/src/client.ts(1 hunks)cli/student/src/plugin.ts(1 hunks)cli/student/src/schema.graphql(1 hunks)demo/Makefile(2 hunks)demo/graph-with-plugin.yaml(1 hunks)demo/pkg/subgraphs/student/.cursor/rules/plugin-development.mdc(1 hunks)demo/pkg/subgraphs/student/.cursorignore(1 hunks)demo/pkg/subgraphs/student/.gitignore(1 hunks)demo/pkg/subgraphs/student/Dockerfile(1 hunks)demo/pkg/subgraphs/student/Makefile(1 hunks)demo/pkg/subgraphs/student/README.md(1 hunks)demo/pkg/subgraphs/student/package.json(1 hunks)demo/pkg/subgraphs/student/src/plugin.ts(1 hunks)demo/pkg/subgraphs/student/src/schema.graphql(1 hunks)router/demo.config.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- cli/src/commands/router/commands/plugin/templates/plugin.ts
🧰 Additional context used
🧬 Code graph analysis (6)
cli/student/src/client.ts (2)
cli/student/generated/service_grpc_pb.d.ts (1)
StudentServiceClient(36-41)cli/student/generated/service_pb.d.ts (1)
QueryHelloRequest(9-21)
cli/src/commands/router/commands/plugin/commands/generate.ts (1)
cli/src/commands/router/commands/plugin/toolchain.ts (5)
getLanguage(668-682)installTsDependencies(531-545)generateProtoAndMapping(382-427)generateGRPCCode(432-489)installGoDependencies(517-529)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
protographic/src/index.ts (2)
compileGraphQLToMapping(16-32)compileGraphQLToProto(53-79)
cli/student/src/plugin.ts (2)
cli/student/generated/service_grpc_pb.d.ts (2)
IStudentServiceServer(26-28)StudentServiceService(24-24)cli/student/generated/service_pb.d.ts (3)
QueryHelloRequest(9-21)QueryHelloResponse(29-44)World(52-66)
demo/pkg/subgraphs/student/src/plugin.ts (2)
demo/pkg/subgraphs/student/generated/service_grpc_pb.d.ts (2)
StudentServiceService(24-24)IStudentServiceServer(26-28)demo/pkg/subgraphs/student/generated/service_pb.d.ts (3)
QueryHelloRequest(9-21)QueryHelloResponse(29-44)World(52-66)
cli/src/commands/router/commands/plugin/commands/build.ts (1)
cli/src/commands/router/commands/plugin/toolchain.ts (7)
getLanguage(668-682)installTsDependencies(531-545)normalizePlatforms(646-666)generateGRPCCode(432-489)installGoDependencies(517-529)buildGoBinaries(598-641)buildTsBinaries(550-593)
🪛 checkmake (0.2.2)
demo/pkg/subgraphs/student/Makefile
[warning] 2-2: Missing required phony target "all"
(minphony)
[warning] 2-2: Missing required phony target "clean"
(minphony)
[warning] 7-7: Target "make" should be declared PHONY.
(phonydeclared)
cli/student/Makefile
[warning] 2-2: Missing required phony target "all"
(minphony)
[warning] 2-2: Missing required phony target "clean"
(minphony)
[warning] 7-7: Target "make" should be declared PHONY.
(phonydeclared)
🪛 GitHub Actions: wgc CI
cli/src/commands/router/commands/plugin/toolchain.ts
[error] 550-550: ESLint: Block must not be padded by blank lines. (padded-blocks). Lint step failed during 'eslint --cache --ext .ts,.mjs,.cjs .'
🪛 GitHub Check: build_test_node_matrix (20.x)
cli/student/src/client.ts
[failure] 38-38:
Prefer top-level await over using a promise chain
[failure] 9-9:
Async function 'run' has no 'await' expression
[failure] 7-7:
Relative imports and exports must end with .js
[failure] 6-6:
Relative imports and exports must end with .js
cli/src/commands/router/commands/plugin/toolchain.ts
[failure] 550-550:
Block must not be padded by blank lines
cli/student/src/plugin.ts
[failure] 4-4:
Prefer node:path over path
[failure] 4-4:
Use default import for module path
[failure] 3-3:
@grpc/grpc-js import should occur after import of url
🪛 GitHub Check: build_test_node_matrix (22.x)
cli/student/src/client.ts
[failure] 38-38:
Prefer top-level await over using a promise chain
[failure] 9-9:
Async function 'run' has no 'await' expression
[failure] 7-7:
Relative imports and exports must end with .js
[failure] 6-6:
Relative imports and exports must end with .js
cli/src/commands/router/commands/plugin/toolchain.ts
[failure] 550-550:
Block must not be padded by blank lines
cli/student/src/plugin.ts
[failure] 4-4:
Prefer node:path over path
[failure] 4-4:
Use default import for module path
[failure] 3-3:
@grpc/grpc-js import should occur after import of url
🪛 markdownlint-cli2 (0.18.1)
demo/pkg/subgraphs/student/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cli/student/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
router/demo.config.yaml (1)
22-22: YAML list indentation corrected.The indentation fix aligns the Redis URL with proper YAML list item formatting under the
urlskey. The change is syntactically correct and improves consistency.cli/student/.cursorignore (1)
1-7: Verify that ignore patterns are also in.gitignore.The
.cursorignorefile is specific to the Cursor IDE and won't be respected by other tools (Git, other IDEs, CI/CD systems). Ensure that these ignore patterns are also present in the appropriate.gitignorefile so that generated artifacts and binaries are properly excluded from version control and other workflows.demo/pkg/subgraphs/student/.gitignore (1)
1-2: Consistent with cli/student/.gitignore pattern.The ignore rule is correct and mirrors the pattern in the parallel CLI file, suggesting a consistent build artifact structure across the plugin scaffolding.
cli/student/Dockerfile (1)
1-22: Dockerfile structure is sound, but clarify multi-language coexistence.The Dockerfile is well-structured for cross-platform Go builds with appropriate use of build cache and scratch image. However, its placement in cli/student/ alongside the Bun package.json (lines 1–22) creates ambiguity. The package.json and Dockerfile both output to
dist/pluginbut use different build systems. Ensure the intended build flow is documented and that both configurations don't conflict or cause confusion about which language/tool to use.demo/graph-with-plugin.yaml (1)
47-50: Configuration looks good.The new student subgraph declaration follows the existing pattern and is properly formatted. Version and path are consistent with the demo/pkg/subgraphs/student/ structure.
cli/student/src/schema.graphql (1)
1-17: Schema design is appropriate for the example.The GraphQL schema is well-structured with clear field documentation. The World type and Query.hello field provide a clean demo implementation. No issues identified.
demo/Makefile (1)
11-11: Integration of student subgraph follows appropriate patterns.The additions to plugin-build (line 11), plugin-generate (line 15), and plugin-build-integration (line 27) correctly mirror the projects subgraph workflow. The absence of
--go-module-pathfor the student subgraph is appropriate since it targets Bun/TypeScript rather than Go. The changes are consistent with the multi-language plugin support introduced in this PR.Also applies to: 15-15, 27-27
cli/src/commands/router/commands/plugin/templates/tsplugin.ts (1)
2-43: Client template: OK to ship.Flows, error handling, and shutdown look fine for a starter template.
cli/src/commands/router/commands/plugin/commands/build.ts (1)
47-62: Language-aware flow looks good; minor cleanups verified as valid.Branching per language, TS deps before codegen, and separate build paths are sound. Verification confirms both optional suggestions:
getHostPlatformis imported at line 13 but never used in build.ts—safe to removeoptions.platformfrom commander's--platform [platforms...]with default[]is alwaysstring[](line 65 passes it tonormalizePlatforms(platforms: string[], ...)), so type safety is guaranteed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/commands/router/commands/plugin/toolchain.ts (1)
23-29: Critical: Windows platform name mismatch breaks TS host builds.
getHostPlatform('ts')returnsbun-${os.platform()}-${arch}, but on Windowsos.platform()returns'win32', not'windows'. TheALL_BUN_PLATFORM_MAPPINGSat lines 43-51 only has keys like'bun-windows-x64', so Windows host builds will fail with "Unsupported platform" whennormalizePlatformstries to default to the host platform.Apply this diff to normalize the platform name:
export function getHostPlatform(language: string) { - const basePlatform = `${os.platform()}-${getOSArch(language)}`; + const nodePlatform = os.platform(); + const normalizedOS = nodePlatform === 'win32' ? 'windows' : nodePlatform; + const basePlatform = `${normalizedOS}-${getOSArch(language)}`; if (language === 'ts') { return `bun-${basePlatform}`; } return basePlatform; }
🧹 Nitpick comments (3)
cli/src/commands/router/commands/plugin/toolchain.ts (2)
656-656: Add.exesuffix and use path join for Windows binary output.Windows executables require the
.exeextension, and the outfile path should usejoin()for cross-platform safety rather than string concatenation.Apply this diff:
- const binaryName = `${platform}_${arch}`; + const binaryName = `${platform}_${arch}${platform === 'windows' ? '.exe' : ''}`; spinner.text = `Building ${originalPlatformArch}...`; if (debug) { const debugScript = resolve(pluginDir, join('bin', originalPlatformArch)); await writeFile(debugScript, pupa(TsTemplates.debugBuild, {})); await chmod(debugScript, 0o755); } else { const flags = [ 'build', 'src/plugin.ts', '--compile', '--outfile', - `bin/${binaryName}`, + join('bin', binaryName), `--target=${originalPlatformArch}`, ];Also applies to: 670-670
614-614: Consider usingbun x tscto ensure TypeScript runs through Bun.Running
bun tscdirectly may respect the#!/usr/bin/env nodeshebang in the TypeScript binary, potentially invoking the system Node.js instead of Bun's runtime. Usingbun x tscensures TypeScript is executed through Bun's package runner.Apply this diff:
- await execa(bunPath, ['tsc', '--noEmit'], { + await execa(bunPath, ['x', 'tsc', '--noEmit'], { cwd: pluginDir, stdout: 'inherit', stderr: 'inherit', env, });cli/src/commands/router/commands/plugin/commands/generate.ts (1)
97-97: Make guidance message language-aware.The success message references
src/main.go, which is Go-specific. TypeScript plugins usesrc/plugin.ts, so this guidance will confuse TS plugin developers.Apply this diff to tailor the message by language:
+ const implFile = language === 'ts' ? 'src/plugin.ts' : 'src/main.go'; + console.log(''); console.log( - ` Now you can modify your implementation in src/main.go, then when you're ready to publish, run ${pc.bold('wgc router plugin publish')}.`, + ` Now you can modify your implementation in ${implFile}, then when you're ready to publish, run ${pc.bold('wgc router plugin publish')}.`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cli/src/commands/grpc-service/commands/generate.ts(3 hunks)cli/src/commands/router/commands/plugin/commands/build.ts(4 hunks)cli/src/commands/router/commands/plugin/commands/generate.ts(4 hunks)cli/src/commands/router/commands/plugin/commands/init.ts(3 hunks)cli/src/commands/router/commands/plugin/toolchain.ts(12 hunks)demo/Makefile(2 hunks)protographic/src/index.ts(1 hunks)protographic/src/sdl-to-proto-visitor.ts(3 hunks)protographic/tests/sdl-to-proto/01-basic-types.test.ts(1 hunks)protographic/tests/sdl-to-proto/10-options.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/src/commands/grpc-service/commands/generate.ts
- protographic/tests/sdl-to-proto/01-basic-types.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-27T09:45:41.622Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.
Applied to files:
protographic/src/index.tsprotographic/src/sdl-to-proto-visitor.ts
📚 Learning: 2025-08-20T10:38:49.191Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.
Applied to files:
protographic/src/sdl-to-proto-visitor.tsprotographic/tests/sdl-to-proto/10-options.test.ts
🧬 Code graph analysis (5)
cli/src/commands/router/commands/plugin/toolchain.ts (2)
protographic/src/index.ts (1)
ProtoOption(98-98)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
protographic/src/sdl-to-proto-visitor.ts (1)
protographic/src/index.ts (1)
ProtoOption(98-98)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
protographic/src/index.ts (2)
compileGraphQLToMapping(16-32)compileGraphQLToProto(53-79)cli/src/commands/router/commands/plugin/toolchain.ts (1)
getGoModulePathProtoOption(597-602)
cli/src/commands/router/commands/plugin/commands/build.ts (3)
cli/src/commands/router/commands/plugin/toolchain.ts (11)
getLanguage(756-769)validateAndGetGoModulePath(267-278)installTsDependencies(583-595)getGoModulePathProtoOption(597-602)normalizePlatforms(735-754)generateProtoAndMapping(417-462)generateGRPCCode(467-525)installGoDependencies(569-581)buildGoBinaries(687-730)typeCheckTs(607-620)buildTsBinaries(625-682)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/commands/generate.ts (3)
cli/src/commands/router/commands/plugin/toolchain.ts (7)
getLanguage(756-769)validateAndGetGoModulePath(267-278)installTsDependencies(583-595)getGoModulePathProtoOption(597-602)generateProtoAndMapping(417-462)generateGRPCCode(467-525)installGoDependencies(569-581)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
🪛 checkmake (0.2.2)
demo/Makefile
[warning] 13-13: Missing required phony target "all"
(minphony)
[warning] 13-13: Missing required phony target "clean"
(minphony)
[warning] 13-13: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build_test
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
protographic/src/sdl-to-proto-visitor.ts (2)
95-105: LGTM! Clean refactor to generic proto options.The new
protoOptionsarray with structuredProtoOptiontype provides better extensibility than the previous singlegoPackagefield. The interface is well-documented with a reference to the proto3 spec.
222-225: LGTM! Option processing logic is correct.The code properly checks for the presence of
protoOptionsand transforms each entry into a valid proto option statement. The verbatim insertion of theconstantfield provides flexibility for different proto constant types (strings, bools, numbers, etc.).protographic/tests/sdl-to-proto/10-options.test.ts (1)
17-24: LGTM! Test correctly validates the new protoOptions API.The test properly uses the new structured
ProtoOptionformat. Theconstantfield contains the correctly quoted string value, which produces valid proto output as confirmed by the inline snapshot at line 32.protographic/src/index.ts (1)
98-98: LGTM! Necessary public API expansion.Adding
ProtoOptionto the exported types enables consumers to use the new structured proto options configuration.cli/src/commands/router/commands/plugin/commands/init.ts (1)
75-77: Disregard this review comment.The review is based on an incorrect assumption. The
PluginTemplates.schemaGraphqltemplate contains only static GraphQL schema content with no placeholders:type World { ... id: ID! ... name: String! ... } type Query { ... hello(name: String!): World! ... }Since the template has no
{name}or any other placeholder variables, there is no inconsistency. PassingPluginTemplates.schemaGraphql(unsubstituted) tocompileGraphQLToMappingis correct—the function receives the intended static schema as-is. Thepupa()call on line 75 has no effect on this template but does not cause issues either.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router-tests/router_plugin_test.go (1)
69-69: Consider adding a similar restart test for the courses subgraph.The rename to "Should restart plugin if it exits for projects" is more specific and accurate. Since you've added courses integration tests below, consider adding a parallel restart test for the courses plugin to ensure its restart behavior is also validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router-tests/router_plugin_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/router_plugin_test.go
🧬 Code graph analysis (1)
router-tests/router_plugin_test.go (1)
router-tests/testenv/testenv.go (2)
GraphQLRequest(1909-1917)Run(105-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (2)
router-tests/router_plugin_test.go (2)
57-66: Good addition of courses subgraph test.Testing both the projects and courses subgraphs when plugins are disabled ensures consistent error handling across both gRPC services.
326-340: Do not approve — tests reference a missing 'courses' subgraphrouter-tests/router_plugin_test.go (lines 326–340) adds three queries against "courses", but the test router config used (ConfigWithPluginsJSONTemplate → router-tests/testenv/testdata/configWithPlugins.json, embedded via router-tests/testenv/testenv.go) does not configure a "courses" subgraph. Add the "courses" subgraph to the test config or change the tests to use a template that includes it; otherwise these tests will not actually validate the courses subgraph and will fail.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
86-86: Remove unnecessary non-null assertion.
goModulePathis defined as a string constant on line 71, so the!operator is unnecessary.Apply this diff:
- protoOptions.push(getGoModulePathProtoOption(goModulePath!)); + protoOptions.push(getGoModulePathProtoOption(goModulePath));router-tests/router_plugin_test.go (2)
106-141: Courses restart test mirrors projects behavior appropriatelyThe new “courses” restart test is a solid companion to the projects one: it kills the courses service, waits for the
plugin process exitedlog, and then asserts a successfulcourses { id }response viarequire.EventuallyWithT. Timeouts and polling intervals are reasonable for CI, and the configuration matches the existing plugin tests.If you see this becoming flaky or slow in CI, consider tightening the
Eventuallywindows once you’ve observed typical restart timings.
363-377: Table-driven courses queries integrate cleanly into existing plugin testsThe added table entries for simple courses listing, single-course lookup, and “employee teaching a course” nicely cover the new demo surface and reuse the existing table-driven harness. The expected JSON bodies are consistent and will strongly assert correct wiring of the new subgraph/plugin.
If more courses-related cases accumulate, consider grouping them (e.g., by prefixing
namewithcourses:) to make it easier to skim which tests exercise which subgraph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/commands/router/commands/plugin/commands/init.ts(3 hunks)router-tests/router_plugin_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/router_plugin_test.go
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router-tests/router_plugin_test.go
🧬 Code graph analysis (2)
router-tests/router_plugin_test.go (1)
router-tests/testenv/testenv.go (7)
GraphQLRequest(1909-1917)Run(105-122)Config(285-342)ConfigWithPluginsJSONTemplate(91-91)LogObservationConfig(388-391)PluginConfig(344-347)Environment(1733-1769)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
protographic/src/index.ts (2)
compileGraphQLToMapping(16-32)compileGraphQLToProto(53-79)cli/src/commands/router/commands/plugin/toolchain.ts (1)
getGoModulePathProtoOption(597-602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
cli/src/commands/router/commands/plugin/commands/init.ts (5)
15-18: LGTM! Clean import additions.The new template and toolchain imports properly support the multi-language plugin scaffolding.
65-69: LGTM! Language validation correctly implemented.The guard properly uses
&&and sets the fallback to'go'when an unsupported language is provided.
106-148: LGTM! Language-specific scaffolding is well-structured.Both Go and TypeScript cases properly generate all necessary files and set template variables. The TypeScript workarounds for Bun compilation issues (lines 135-142) are well-documented and align with the PR description.
150-187: LGTM! Project and non-project modes handle scaffolding correctly.Both modes properly generate the necessary files and directory structure, using the
mainFileNameandreadmeTemplatevariables set during language-specific scaffolding. The final atomic rename on line 187 is a good pattern to avoid partial state on failure.
75-97: The review comment is incorrect. TheschemaGraphqltemplate contains no placeholders.The template is a static GraphQL schema with no
{name}or other interpolation placeholders. Therefore,pupa(PluginTemplates.schemaGraphql, { name })on line 75 produces identical output to the raw template, making it safe forcompileGraphQLToMapping(line 76) andcompileGraphQLToProto(line 91) to use the unrendered template. The code is correct.Likely an incorrect or invalid review comment.
router-tests/router_plugin_test.go (2)
57-65: Good extension of negative-path coverage for newcoursessubgraphAdding the
course(id: 1)query alongsideproject(id: 1)ensures the “plugin disabled” behavior is asserted for both gRPC-backed subgraphs with the same error shape. This is consistent with the existing pattern and looks correct.
69-104: Projects restart test remains sound after renameThe renamed subtest still exercises “kill → observe exit log → successful projects query” and the
require.EventuallyWithTusage plus strict body assertion give good confidence the plugin restarts correctly. No issues from the change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
86-86: Remove unnecessary non-null assertion.The
goModulePathvariable is defined as a string literal at line 71 and is never null or undefined, so the non-null assertion operator!is unnecessary.Apply this diff:
- protoOptions.push(getGoModulePathProtoOption(goModulePath!)); + protoOptions.push(getGoModulePathProtoOption(goModulePath));demo/pkg/subgraphs/courses/src/plugin.ts (5)
1-30: Keep demo package.json in sync with gRPC/protobuf dependenciesThis module (and
plugin-server.ts) rely on@grpc/grpc-js,google-protobuf(wrappers),grpc-health-check, and the generatedservice_*modules. Please ensuredemo/pkg/subgraphs/courses/package.jsondeclares these as dependencies so Bun can build and run the plugin reliably (especially in CI/Docker).
32-62: ID generator is concurrency-safe but more complex than neededUsing
SharedArrayBuffer+Atomicsis robust, but given handlers execute on a single JS thread in Bun/Node, a simple module-level counter (e.g.let nextId = 1000;) would be enough and easier to reason about. Consider simplifying if you don’t need true cross-thread atomics.
215-283: Clarify intent ofqueryThrowErrorCoursesand error handling
queryThrowErrorCoursesthrows directly from the handler and never calls the callback. Depending on@grpc/grpc-jsbehavior, this may crash the Bun/Node process rather than just surfacing an application error.If the goal is to simulate a failing RPC while keeping the plugin alive, prefer
callback(new Error('...')). If the goal is explicitly to crash the plugin for router resilience tests, consider documenting that intent here so future readers don’t “fix” it unintentionally.
285-341: Mutations accept invalid input without signaling errorsBoth
mutationAddCourseandmutationAddLessonhappily proceed with whatever IDs/titles they receive (e.g. unknowncourseId, emptytitle), silently creating partial or dangling data.That’s fine for a demo, but if you plan to reuse this as a template, consider adding minimal validation (and returning a gRPC error) so downstream behavior is more predictable.
410-419: Separate plugin implementation from server startup to avoid import side effectsRight now
plugin.tsboth exportspluginImplementationand, at module load, instantiatesPluginServer, registers the service, and calls.serve(). That’s convenient for the binary entrypoint, but it means any import of this module (e.g. unit tests) will also start a gRPC server as a side effect.Consider splitting this into:
plugin.ts: exports onlypluginImplementation.main.ts(or similar): importspluginImplementation+PluginServer, wires the service, and callsserve().This keeps tests and tooling free of unexpected server startup while still giving you a clean entrypoint for the Bun-built plugin binary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
demo/pkg/subgraphs/courses/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
cli/src/commands/router/commands/plugin/commands/init.ts(3 hunks)cli/src/commands/router/commands/plugin/templates/typescript.ts(1 hunks)cli/src/commands/router/commands/plugin/templates/typescript/package.json.template(1 hunks)cli/src/commands/router/commands/plugin/templates/typescript/plugin-server.ts.template(1 hunks)cli/src/commands/router/commands/plugin/templates/typescript/protobufjs_inquire_patch.template(1 hunks)demo/pkg/subgraphs/courses/package.json(1 hunks)demo/pkg/subgraphs/courses/patches/@[email protected](1 hunks)demo/pkg/subgraphs/courses/src/plugin-server.ts(1 hunks)demo/pkg/subgraphs/courses/src/plugin.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cli/src/commands/router/commands/plugin/templates/typescript/package.json.template
🚧 Files skipped from review as they are similar to previous changes (3)
- demo/pkg/subgraphs/courses/src/plugin-server.ts
- demo/pkg/subgraphs/courses/package.json
- cli/src/commands/router/commands/plugin/templates/typescript/plugin-server.ts.template
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
🧬 Code graph analysis (2)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
protographic/src/index.ts (2)
compileGraphQLToMapping(16-32)compileGraphQLToProto(53-79)cli/src/commands/router/commands/plugin/toolchain.ts (1)
getGoModulePathProtoOption(597-602)
demo/pkg/subgraphs/courses/src/plugin.ts (3)
demo/pkg/subgraphs/courses/generated/service_pb.d.ts (23)
Course(495-523)Employee(576-592)Lesson(535-563)QueryCoursesRequest(202-212)QueryCoursesResponse(219-233)QueryCourseRequest(241-253)QueryCourseResponse(261-276)QueryLessonsRequest(284-296)QueryLessonsResponse(304-318)QueryKillCoursesServiceRequest(326-336)QueryKillCoursesServiceResponse(343-355)QueryThrowErrorCoursesRequest(363-373)QueryThrowErrorCoursesResponse(380-392)MutationAddCourseRequest(400-414)MutationAddCourseResponse(423-438)MutationAddLessonRequest(446-462)MutationAddLessonResponse(472-487)LookupCourseByIdRequest(30-44)LookupCourseByIdResponse(52-66)LookupLessonByIdRequest(94-108)LookupLessonByIdResponse(116-130)LookupEmployeeByIdRequest(158-172)LookupEmployeeByIdResponse(180-194)demo/pkg/subgraphs/courses/src/plugin-server.ts (1)
PluginServer(10-69)demo/pkg/subgraphs/courses/generated/service_grpc_pb.d.ts (1)
CoursesServiceService(115-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (10)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
132-140: Well-documented workaround for protobuf.js bundling issue.The comments clearly explain the grpc-health-check
__dirnameissue and the rationale for the patches. The note about potential breakage in Cosmo Cloud builds when Docker build directories are temporary is a valuable warning for future maintainers.
75-77: Review comment is incorrect. The premise assumesPluginTemplates.schemaGraphqlcontains placeholders like{name}, but verification confirms the template contains only hardcoded GraphQL schema with no placeholders. Consequently,pupa()at line 75 returns the template unchanged, and all three usages at lines 75, 76, and 90 receive identical content. There is no inconsistency or risk of incorrect code generation.Likely an incorrect or invalid review comment.
demo/pkg/subgraphs/courses/patches/@[email protected] (1)
1-50: LGTM: Safe replacement of eval-based module loading.The patch replaces protobufjs's eval trick with a safer direct
requirecall while maintaining the same fallback behavior. The addition of proper guards (typeof require !== "function") and better error handling improves robustness for bundled environments like Bun.The upstream reference and explanation in the comments are helpful for future maintainers.
cli/src/commands/router/commands/plugin/templates/typescript/protobufjs_inquire_patch.template (1)
1-50: LGTM: Template source for protobufjs inquire patch.This template correctly mirrors the patch applied in the demo subgraph and will be used to generate the same patch for new TypeScript plugins during initialization.
cli/src/commands/router/commands/plugin/templates/typescript.ts (3)
1-4: Auto-generated templates module.The file header clearly indicates this is auto-generated, which is good for maintainability. Ensure the generation script (referenced in the AI summary as
cli/scripts/compile-templates.ts) is properly documented and that changes to templates are made in the source template files rather than this generated output.
17-18: Dependencies configuration looks appropriate.The
packageJsontemplate includes the necessary gRPC dependencies and properly declares the two patches inpatchedDependencies. The patches address the protobufjs inquire issue and the grpc-health-check__dirnameissue documented elsewhere in the PR.
38-50: LGTM: Clean template exports.The default export provides a well-structured object with all necessary templates for TypeScript plugin scaffolding. The naming is consistent and the templates are properly organized.
demo/pkg/subgraphs/courses/src/plugin.ts (3)
63-148: Sample data wiring looks consistentEmployee
taughtCourseIds, courselessonIds, and thelessonsmap are aligned, and generated IDs start at1000so they won’t collide with the hardcoded demo IDs. This in-memory seed state looks coherent.
153-213: Converters correctly map domain data to protobuf messages
courseDataToCourse,lessonDataToLesson, andemployeeDataToEmployeecorrectly set required fields, only populateStringValuedescriptions when present, and use stub references for relatedCourse/Employeeto avoid recursive expansion. This mapping looks solid.
343-381: Course/Lesson federation lookup implementations look correct
lookupCourseByIdandlookupLessonByIditerate the request keys, look up entities from the in-memory Maps, and push converted messages for found entries only. Duplicate keys yield duplicate results, which matches common entity-lookup semantics. No issues here.
|
FYI: Discussed with Dustin that we only install tools specific per language (right now we install bun when using go plugins), will update and push soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (4)
cli/src/commands/router/commands/plugin/toolchain.ts (3)
31-52: TS binary build: use joined outfile path and.exeon Windows targets
buildTsBinariesmostly looks good, but there are a couple of correctness/robustness gaps:
- Outfile path is passed as a plain string:
'--outfile', `bin/${binaryName}`rather thanjoin(binDir, binaryName). This is brittle and bypasses your earlierbinDircalculation.- Windows targets currently output
windows_amd64without a.exeextension. Depending on how the router spawns the binary, this can make the artifact unusable on Windows hosts.You can tighten this up with:
const [platform, arch] = platformArch.split('-'); if (!platform || !arch) { throw new Error( `Invalid platform-architecture format: ${originalPlatformArch}. Use format like 'bun-darwin-arm64'`, ); } const binaryName = `${platform}_${arch}${platform === 'windows' ? '.exe' : ''}`; const outfile = join(binDir, binaryName); … const flags = [ 'build', 'src/plugin.ts', '--compile', '--outfile', outfile, `--target=${originalPlatformArch}`, ];This keeps naming consistent and avoids path bugs across OSes.
Also applies to: 35-41, 653-671, 672-711
22-29: Windows TS host platform mismatch (bun-win32-x64vsbun-windows-x64)
getHostPlatform('ts')buildsbun-${os.platform()}-${getOSArch('ts')}. On Windows this yieldsbun-win32-x64, but:
ALL_BUN_PLATFORMSandALL_BUN_PLATFORM_MAPPINGSusebun-windows-x64.normalizePlatformsusesgetHostPlatform(language)as the default platform.Result:
normalizePlatforms([], false, 'ts')on Windows will produce['bun-win32-x64'], which has no mapping and later fails inbuildTsBinaries.Normalize the OS name the same way everywhere, e.g.:
export function getHostPlatform(language: string) { const nodePlatform = os.platform(); const normalizedOS = nodePlatform === 'win32' ? 'windows' : nodePlatform; const basePlatform = `${normalizedOS}-${getOSArch(language)}`; if (language === 'ts') { return `bun-${basePlatform}`; } return basePlatform; }This keeps TS host defaults consistent with
ALL_BUN_PLATFORMSand the Bun mapping table.Also applies to: 132-142, 761-783, 785-797
633-649: Run TS type-checks viabun x tsc --noEmitinstead ofbun tsc
typeCheckTscurrently does:await execa(bunPath, ['tsc', '--noEmit'], { ... });This invokes the
tscscript directly, which still uses the Node shebang and can hit the system Node (or fail if Node is missing/too old), undermining the Bun‑only toolchain goal.Prefer Bun’s runner to execute the package binary:
await execa(bunPath, ['x', 'tsc', '--noEmit'], { cwd: pluginDir, stdout: 'inherit', stderr: 'inherit', env, });This ensures the TypeScript CLI comes from the project dependencies and runs under Bun.
Is `bun x tsc --noEmit` the recommended way to run TypeScript type-checks under Bun without relying on a global Node installation?cli/src/commands/router/commands/plugin/commands/generate.ts (1)
38-45: Make success messaging and details language‑aware (Go vs TS)You already detect
languageand branch logic, but the user‑facing output is still Go‑specific:
- Guidance always says: “modify your implementation in
src/main.go…”, which is wrong for TS plugins (src/plugin.ts).- Details don’t show the language at all, and
protoOptionsis the only extra field.Consider:
- Adding
languageto the details map.- Picking an implementation hint file per language:
const implHintFile = language === 'ts' ? 'src/plugin.ts' : 'src/main.go'; const details: Record<string, string> = { output: pluginDir, time: formattedTime, language, protoOptions: protoOptionsSummary, }; … console.log( ` Now you can modify your implementation in ${implHintFile}, then when you're ready to publish, run ${pc.bold( 'wgc router plugin publish', )}.`, );This clarifies next steps for TS users without affecting Go.
Also applies to: 76-82, 95-98
🧹 Nitpick comments (2)
scripts/install-proto-tools.sh (1)
9-12: LANGUAGE underset -ucan fail with an unhelpful “unbound variable” errorWith
set -u,LANGUAGE="${LANGUAGE}"will abort the script ifLANGUAGEis unset (e.g. when someone curls the script manually), before reaching the niceUnsupported language: $LANGUAGE. Must be 'go' or 'ts'.error.Consider guarding or defaulting
LANGUAGEexplicitly so failures are clearer:# Example: require LANGUAGE with a helpful message : "${LANGUAGE:?LANGUAGE must be set to 'go' or 'ts'}"or provide a sane default like
LANGUAGE="${LANGUAGE:-go}"if that matches your intended UX.Also applies to: 236-249
cli/src/commands/router/commands/plugin/commands/test.ts (1)
34-41: Simplify test failure handling;result.failedis never true with default execa optionsInside the inner test
tryyou do:let failed = false; switch (language) { case 'go': { const result = await runGoTests(srcDir, spinner, false); failed = result.failed; break; } case 'ts': { const result = await runTsTests(pluginDir, spinner); failed = result.failed; break; } } … const title = failed ? 'Tests failed!' : 'Tests completed successfully!';But with execa’s default
reject: true, a non‑zero exit throws instead of returning{ failed: true }, so:
- On success:
failedstaysfalse, title = “Tests completed successfully!”- On failure: control jumps to the
catchblock and this code path is never reached.You can drop
failedentirely and treat reaching thetryblock as success:await (language === 'go' ? runGoTests(srcDir, spinner, false) : runTsTests(pluginDir, spinner)); const title = 'Tests completed successfully!'; // render success treeand rely on the existing
catchto render the failure tree.Also applies to: 67-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/src/commands/router/commands/plugin/commands/build.ts(3 hunks)cli/src/commands/router/commands/plugin/commands/generate.ts(4 hunks)cli/src/commands/router/commands/plugin/commands/test.ts(2 hunks)cli/src/commands/router/commands/plugin/toolchain.ts(20 hunks)scripts/install-proto-tools.sh(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T10:38:49.191Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.
Applied to files:
cli/src/commands/router/commands/plugin/commands/build.ts
🧬 Code graph analysis (4)
cli/src/commands/router/commands/plugin/commands/generate.ts (4)
cli/src/commands/router/commands/plugin/toolchain.ts (8)
getLanguage(785-798)checkAndInstallTools(307-356)validateAndGetGoModulePath(291-302)installTsDependencies(612-624)getGoModulePathProtoOption(626-631)generateProtoAndMapping(446-491)generateGRPCCode(496-554)installGoDependencies(598-610)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)protographic/src/index.ts (1)
ProtoOption(98-98)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/commands/build.ts (3)
cli/src/commands/router/commands/plugin/toolchain.ts (12)
getLanguage(785-798)checkAndInstallTools(307-356)validateAndGetGoModulePath(291-302)installTsDependencies(612-624)getGoModulePathProtoOption(626-631)normalizePlatforms(764-783)generateProtoAndMapping(446-491)generateGRPCCode(496-554)installGoDependencies(598-610)buildGoBinaries(716-759)typeCheckTs(636-649)buildTsBinaries(654-711)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/toolchain.ts (2)
protographic/src/index.ts (1)
ProtoOption(98-98)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/commands/test.ts (2)
cli/src/commands/router/commands/plugin/toolchain.ts (6)
getLanguage(785-798)checkAndInstallTools(307-356)installGoDependencies(598-610)installTsDependencies(612-624)runGoTests(559-577)runTsTests(579-593)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cli/src/commands/router/commands/plugin/commands/generate.ts (1)
97-97: Make guidance message language-aware.The success message still references
src/main.gofor all languages, but TS plugins usesrc/plugin.ts. Tailor the message by language.Apply:
+ const implHintFile = language === 'ts' ? 'src/plugin.ts' : 'src/main.go'; console.log( - ` Now you can modify your implementation in src/main.go, then when you're ready to publish, run ${pc.bold('wgc router plugin publish')}.`, + ` Now you can modify your implementation in ${implHintFile}, then when you're ready to publish, run ${pc.bold('wgc router plugin publish')}.`, );cli/src/commands/router/commands/plugin/toolchain.ts (2)
23-29: Windows host platform mismatch breaks TS builds.
getHostPlatform('ts')returnsbun-win32-x64(fromos.platform()), butALL_BUN_PLATFORM_MAPPINGSkeys usebun-windows-x64. This causes platform normalization to fail on Windows.Apply:
export function getHostPlatform(language: string) { + const nodePlatform = os.platform(); + const normalizedOS = nodePlatform === 'win32' ? 'windows' : nodePlatform; - const basePlatform = `${os.platform()}-${getOSArch(language)}`; + const basePlatform = `${normalizedOS}-${getOSArch(language)}`; if (language === 'ts') { return `bun-${basePlatform}`; } return basePlatform; }
736-750: Add.exeextension for Windows and use safer path construction.Two issues remain from past review:
- Windows binaries need
.exeextension- Output path should use
join()instead of string concatenationApply:
- const binaryName = `${platform}_${arch}`; + const binaryName = `${platform}_${arch}${platform === 'windows' ? '.exe' : ''}`; @@ const flags = [ 'build', 'src/plugin.ts', '--compile', '--outfile', - `bin/${binaryName}`, + join('bin', binaryName), `--target=${originalPlatformArch}`, ];
🧹 Nitpick comments (1)
scripts/install-proto-tools.sh (1)
13-13: Clarify LANGUAGE variable assignment or add validation.Line 13 assigns
LANGUAGE="${LANGUAGE}", which is a tautology—if LANGUAGE is unset, it remains empty. While the case statement at line 276 correctly errors out on empty LANGUAGE, the assignment at line 13 suggests a default will be applied, which is misleading. Either remove the assignment and rely on the case-statement error handling, or add a comment explaining that LANGUAGE must be explicitly provided via the environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/src/commands/router/commands/plugin/commands/build.ts(3 hunks)cli/src/commands/router/commands/plugin/commands/generate.ts(4 hunks)cli/src/commands/router/commands/plugin/toolchain.ts(17 hunks)scripts/install-proto-tools.sh(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T10:38:49.191Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.
Applied to files:
cli/src/commands/router/commands/plugin/commands/build.ts
🧬 Code graph analysis (3)
cli/src/commands/router/commands/plugin/commands/build.ts (2)
cli/src/commands/router/commands/plugin/toolchain.ts (12)
getLanguage(836-849)checkAndInstallTools(357-406)validateAndGetGoModulePath(341-352)installTsDependencies(663-675)getGoModulePathProtoOption(677-682)normalizePlatforms(815-834)generateProtoAndMapping(497-542)generateGRPCCode(547-605)installGoDependencies(649-661)buildGoBinaries(767-810)typeCheckTs(687-700)buildTsBinaries(705-762)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/commands/generate.ts (3)
cli/src/commands/router/commands/plugin/toolchain.ts (8)
getLanguage(836-849)checkAndInstallTools(357-406)validateAndGetGoModulePath(341-352)installTsDependencies(663-675)getGoModulePathProtoOption(677-682)generateProtoAndMapping(497-542)generateGRPCCode(547-605)installGoDependencies(649-661)cli/src/commands/router/commands/plugin/helper.ts (1)
renderResultTree(13-67)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
cli/src/commands/router/commands/plugin/toolchain.ts (2)
protographic/src/index.ts (1)
ProtoOption(98-98)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
🔇 Additional comments (25)
scripts/install-proto-tools.sh (1)
186-237: LGTM! Well-structured download functions with consistent error handling.The
download_bun()anddownload_node()functions follow the established patterns in the script, properly extract and place binaries, and include appropriate symlink creation for Node.js binaries. Error handling is consistent with existing download functions.cli/src/commands/router/commands/plugin/commands/generate.ts (5)
38-44: LGTM!Language detection with early-exit pattern is clean and user-friendly.
46-68: LGTM!Language-aware setup is correctly sequenced: TS installs dependencies early (needed for protoc plugins), while Go accumulates proto options for generation.
71-74: LGTM!Updated function signatures correctly match the language-aware toolchain API.
76-81: LGTM!Go dependency installation correctly placed after GRPC code generation (required for
go mod tidyto succeed).
92-92: LGTM!ProtoOptions now render correctly as human-readable strings. This addresses the previous review feedback.
Also applies to: 103-103
cli/src/commands/router/commands/plugin/commands/build.ts (5)
50-56: LGTM!Language detection with early-exit is consistent with the generate command.
58-84: LGTM!Language-aware setup and platform normalization correctly sequenced and consistent with the generate command.
87-90: LGTM!Function calls correctly use the updated language-aware signatures.
93-104: LGTM!Language-specific build paths are cleanly separated, and TS type-checking before build is good practice.
120-120: LGTM!ProtoOptions rendering is correct and addresses previous review feedback.
Also applies to: 130-130
cli/src/commands/router/commands/plugin/toolchain.ts (14)
31-51: LGTM!Platform mappings are comprehensive and the musl preference is well-documented.
64-131: LGTM!Language-specific tool configurations are correctly keyed by language (
'go','ts') and include appropriate tools for each runtime. Past review feedback regarding key mismatch has been addressed.
140-150: LGTM!Arch mapping correctly handles Go's
amd64vs standardx64convention.
155-252: LGTM!Reinstall logic correctly distinguishes between common tool updates (requiring cleanup) and new language-specific tools (no cleanup), preventing unnecessary reinstalls.
257-284: LGTM!Host tool detection is language-aware with clear error messages.
341-352: LGTM!Go module path validation correctly prevents misuse for non-Go languages.
357-406: LGTM!Tool installation prompts are language-aware and user-friendly.
427-492: LGTM!Tool installation correctly handles language-specific toolchains and cleanup logic.
497-542: LGTM!Proto generation correctly uses the ProtoOption array for language-agnostic configuration.
547-605: LGTM!Language-specific GRPC code generation correctly handles different protoc plugin configurations with good error handling.
630-682: LGTM!TS test runner, dependency installer, and ProtoOption helper are correctly implemented.
687-700: LGTM!Type-checking correctly uses
bun x tscto run TypeScript through Bun's runtime.
815-834: LGTM!Platform normalization is language-aware and avoids identifier shadowing.
836-849: LGTM!Language detection logic is straightforward and correct.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
This PR implements typescript plugins with bun.
There are no changes in the router, because we build a binary in the same name format it expects
{platform}-{arch}This PR also removes the template TS constants into template files, which are appended into the previous constants dynamically. This makes it easier to manage a growing number of templates, instead of going through TS strings. Includes a CI check to verify that any template changes are generated and committed.
To keep thing simple we didn't add a separate folder for the ts router plugin
router-plugin-ts, this will be done in a follow up pr, after everything is approved.We used
bun patchto override the internal__dirnamebecause it is hardcoded to the directory the binary was compiled at on compile time (this especially breaks cosmo cloud plugins since the docker directory is temporary for building). Also we bun patch theprotobufjs/inquiremodule as it has not been updated due to a build issue on their repository.Adds courses demo
Summary by CodeRabbit
New Features
Chores
Checklist